SQL: Test and fix the NULL handling of the String functions #68379

palesz · 2021-02-02T14:23:24Z

Fixed the inconsistencies regarding NULL argument handling.
NULL literal vs NULL field value as function arguments in some case
resulted in different function return values.

Functions should return with the same value no matter if the argument(s)
came from a field or from a literal.

The introduced integration test tests if function calls with same
argument values (regardless of literal/field) will return with the
same output (also checks if newly added functions are checked
by the test).

Fixed the following functions:

Insert: NULL start, length and replacement arguments (as fields) also
result in NULL return value instead of returning the input.
Locate: NULL pattern results in NULL return value
Replace: NULL pattern and replacement results in NULL instead of
returning the input
Substring: NULL start or length results in NULL instead of returning
the input

Fixes #58907

Fixed the inconsistencies regarding NULL argument handling. NULL literal vs NULL field value as function arguments in some case resulted in different function return values. Functions should return with the same value no matter if the argument(s) came from a field or from a literal. The introduced integration test tests if function calls with same argument values (regardless of literal/field) will return with the same output (also checks if newly added functions are added to the testcases). Fixed the following functions: * Insert: NULL start, length and replacement arguments (as fields) also result in NULL return value instead of returning the input. * Locate: NULL pattern results in NULL return value, NULL start parameter results 0 return value (0 = no match) instead of NULL * Replace: NULL pattern and replacement results in NULL instead of returning the input * Substring: NULL start or length results in NULL instead of returning the input Fixes elastic#58907

elasticmachine · 2021-02-02T14:23:28Z

Pinging @elastic/es-ql (Team:QL)

palesz · 2021-02-02T14:38:36Z

x-pack/plugin/sql/qa/server/src/main/resources/functions.csv-spec

+false          |83             
+true           |13             


Especially curious about your opinions on this one:

These changes are the result of start=NULL{f} argument handling (this is an explicit start=NULL vs leaving out the optional parameter).

Previously:
LOCATE(pattern,input,start=NULL{field}) was handled as LOCATE(pattern,input,start=1) or LOCATE(pattern, input), but LOCATE(pattern,input,start=NULL{literal}) resulted in NULL.

Now in both cases we return with 0 to be in-sync with the MySQL behaviour (where LOCATE comes from).

Possible alternative approach: to stay backwards-compatible and not change query results where the start comes from a field, make LOCATE(pattern,input,start=NULL{literal}) behave the as LOCATE(pattern,input).

Note:
MySQL behaviour: LOCATE(pattern,input,start=NULL) => 0
MS SQL behaviour: CHARINDEX(pattern,input,start=NULL) => NULL

Overall I think it would follow the MySQL behaviour since LOCATE comes from there.
If we had to change the behaviour only for a literal NULL I would say to go ahead and provide it as a bug fix, but since now we changes how it behaves for a NULL coming from a field, we should really consider. I guess we can always declare it a breaking change?

I reverted to the alternative approach and made the LOCATE(pattern,input,start=NULL{literal}) mimic the current behaviour of the LOCATE(pattern,input,start=NULL{field}) in this PR. Our LOCATE also handles the negative start values differently from MySQL. start=missing/NULL/negative value all result the same as start=1 (tries to find the pattern in the whole input) while MySQL considers start=negative value/NULL as invalid and returns with 0 (no match). Because LOCATE treats NULL as the start argument specially even in MySQL (not the same as any other argument being NULL) I will address these differences in a separate PR.

I am still adding the breaking change label to this PR, since the behaviour of the functions change anyways (NULL field values will start returning NULL values instead of the original input strings).

mark-vieira · 2021-02-03T00:15:49Z

@elasticmachine update branch

costin · 2021-02-03T07:51:37Z

...rc/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

+ * <p>To ignore any of the tests, add an .ignore() method call after the Fn ctors in the FUNCTION_CALLS_TO_TEST list below, like:
+ * <code> new Fn("ASCII", "foobar").ignore()</code></p>
+ */
+public class ConsistentFunctionArgHandlingIT extends JdbcIntegrationTestCase {


+1 for generating the test case to test various combinations however I find the current approach too convoluted.
Instead of indexing the data, filtering the functions and generating queries through an integration test one could use the FunctionRegistry directly and just focus on the string functions either by declaring their arguments as in this test or inspecting the returned definitions.

Specify the arguments or generate the loops based on the signature and pass them to the definition to get back the actual function instance to fold.

The resulting code should be smaller, without strings and faster due to be being a unit test.

I agree that writing a unit test would result in faster tests, however testing if:

processors

Function.fold() (optimizer folding rule that calls that if all args are foldable, calls the processor)

optimizer rules that rewrite the function call in the query (NULL folding for example that depends on Function.nullable())

QueryTranslator output (function calls turn into painless scripts that call the processors within Query DSL queries)

all work together in a consistent way providing the same outcome regardless the argument sources being field/literal is easier through integration tests, because the different argument source combinations will hit all of the above code paths using the same test code.

I agree though that in unit tests like LocateProcessorTests (or one similar to this in the PR) we could easily test if fold() and the processor leads to the same results, and likely it'll catch some of the bugs. But it will not cover #3 and #4.

To cover #3 we would have to make sure we call the Optimizer too, check if the function transformed by the rules will provide an output equivalent to fold and processor calls.
To cover #4 we would have to execute the query generated by the QueryTranslator (or at least the generated Painless script), which I think is easier to do through integration tests and would require quite a bit of mocking in a unit test.

This looks too much of a whitebox testing for me. I'd rather execute the same code that the user's code would execute with less assumptions about our implementation. IMO it is easier to argue about the coverage of this integration test that uses the same approach to hit all the four cases above.

Discovery: SqlFunctionRegistry is not accessible in integration test (not impossible to fix), but even if it would be (or would want to use in a unit test): I don't see how can I easily figure out the type of the arguments from the FunctionDefinition. The constructor arguments of the functions are Expressions without hints about the actual accepted types and the Builders also don't provide help. One way is to construct a function with generic/widely unacceptable arguments, call resolveType() and parse the error messages. It would help if the functions would be self-decribing (we could even generate a function arguments meta table with type, optional, accepted sources per function that might be useful somewhere else), but it is outside of the scope of this PR.

costin · 2021-02-03T07:57:23Z

.../sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/string/Locate.java

@@ -80,6 +81,11 @@ protected Pipe makePipe() {
        return NodeInfo.create(this, Locate::new, pattern, input, start);
    }

+    @Override
+    public Nullability nullable() {
+        return Nullability.FALSE;


If the function returns null if any argument is null, this should be TRUE not FALSE.

LOCATE does not return NULL if start=NULL.

What about the other parameters?

If any of the pattern or input is NULL, the result is NULL. Only start is special.

Then the function is nullable as oppose to never nullable. Depending on the params (outside start) it can TRUE or UKNOWN - see Nullability.and.

You are right, changed.

costin · 2021-02-03T08:02:59Z

Not only the functions need to be checked but also the underlying processors which are called for performing extractions or working on painless. This is needed to make sure that passing nulls to processors works the same as passing nulls to the functions.

While the processors are tested separately, it's worth looking into a test that checks both with the same set of arguments. At that point, manually declaring the functions and the processors plus their args might be easier than going for the discovery approach.

matriv · 2021-02-03T15:15:25Z

@palesz With the opportunity of this fix, please adjust the docs for those functions, where currently seem to not mention (apart from LOCATE) the edge cases with null args.

…spotless happy and the code short)

…l-sql-fn-null

palesz · 2021-02-03T21:44:48Z

@palesz With the opportunity of this fix, please adjust the docs for those functions, where currently seem to not mention (apart from LOCATE) the edge cases with null args.

See 7b8de11 of this PR. Clarified the edge cases on CONCAT and LOCATE (current behavior kept in this PR). The other functions don't have edge cases, any NULL args will return NULL, so left their doc untouched.

astefan

Left one question mainly, the second one can be left unanswered as I'm not too convinced the alternative is better.
@costin has a point about the IT tests being maybe a bit too much for what they are achieving. There are ~500 queries being executed when the tests run. If the impact on the execution time is not too much, I'd keep them.
Otherwise LGTM.

astefan · 2021-02-03T21:30:09Z

...rc/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

+            this.name = name;
+            this.arguments = new ArrayList<>();
+            for (Object a : arguments) {
+                this.arguments.add(Argument.class.isAssignableFrom(a.getClass()) ? (Argument) a : new Argument(a));


When can there already be an Argument sent as a parameter to this constructor? I am only seeing non-Argument instances being used....

Previously I used for one of the cases (LOCATE). Now this is unused and can be removed, but might come handy if we have a restriction that some of the arguments cannot come from fields.

@palesz I would suggest removing it and use it when needed. Someone looking at the code gets confused about a functionality that's not actually there.

astefan · 2021-02-03T21:50:32Z

...rc/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

+            // first set the same value, so the mapping is populated for the null columns
+            testDoc.put(nullArgPrefix + idx, arg.exampleValue);
+        }
+        index(indexName, functionName, body -> body.mapContents(testDoc));


Is it easier (would definitely be less indexing) to use a mapping to create the index rather than having the index mapping be auto-detected by ES at the first document being indexed?
On the other hand, using a fixed mapping is kind of messy... there are a lot of fields in it.

Was thinking along the same lines. It would definitely be more code.

matriv · 2021-02-04T09:54:39Z

@palesz With the opportunity of this fix, please adjust the docs for those functions, where currently seem to not mention (apart from LOCATE) the edge cases with null args.

See 7b8de11 of this PR. Clarified the edge cases on CONCAT and LOCATE (current behavior kept in this PR). The other functions don't have edge cases, any NULL args will return NULL, so left their doc untouched.

Thx! Though, myself, I'd prefer to explicitly state what's the behaviour with NULL in all cases, like we do for example in: https://www.elastic.co/guide/en/elasticsearch/reference/current/sql-functions-datetime.html#sql-functions-datetime-datetimeparse:

Description: Returns a datetime by parsing the 1st argument using the format specified in the 2nd argument.
The parsing format pattern used is the one from java.time.format.DateTimeFormatter. 
*If any of the two arguments is null or an empty string null is returned*.

costin

LGTM

palesz · 2021-02-04T16:13:55Z

@costin has a point about the IT tests being maybe a bit too much for what they are achieving. There are ~500 queries being executed when the tests run.

@astefan See my previous comment #68379 (comment) about the reasoning behind integration tests.

If the impact on the execution time is not too much, I'd keep them.

The impact on execution time is about ~13 seconds. This test runs as part of the ci/2 that takes ~60 minutes.

palesz · 2021-02-04T16:15:22Z

Thx! Though, myself, I'd prefer to explicitly state what's the behaviour with NULL in all cases

As discussed, will handle in #68531

bpintea

LGTM.

bpintea · 2021-02-04T16:20:10Z

...a/org/elasticsearch/xpack/sql/expression/function/scalar/string/InsertFunctionProcessor.java

-            return input;
-        }
-        if ((replacement instanceof String || replacement instanceof Character) == false) {
+        if (!(replacement instanceof String || replacement instanceof Character)) {


Nit: was the change intentional (here and one more spot in the patch)?
(Myself I dislike == false, but since this the direction the train is going, I'm comfier with a consistent style.)

Thanks for pointing out. No, it wasn't, I think just IDEA tried to be helpful (will revert this one).

astefan · 2021-02-04T17:42:51Z

...rc/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

+            this.name = name;
+            this.arguments = new ArrayList<>();
+            for (Object a : arguments) {
+                this.arguments.add(Argument.class.isAssignableFrom(a.getClass()) ? (Argument) a : new Argument(a));


@palesz I would suggest removing it and use it when needed. Someone looking at the code gets confused about a functionality that's not actually there.

astefan · 2021-02-04T17:45:41Z

...rc/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

                assertTrue(retVal.next());
                results.add(tuple(functionName + "(" + join(", ", functionCallArgsForAssert) + ")", retVal.getObject(1)));
                // only a single row should be returned
                assertFalse(retVal.next());

-                Stream<Object> returnValueStream = results.stream().map(Tuple::v2);


Why did you remove this and now go over the stream components twice?

Sadly streams can only be iterated once, during the second iteration it gave stream is already closed exception.

Why did the tests pass then?

Because the second iteration only happens if the test finds an issue (to create the detailed message for error).

matriv

LGTM

matriv · 2021-02-05T12:35:33Z

...rc/test/java/org/elasticsearch/xpack/sql/qa/single_node/ConsistentFunctionArgHandlingIT.java

+    static {
+        try {
+            Class<?> c = ConsistentFunctionArgHandlingIT.class;
+            NON_TESTED_FUNCTIONS = Files.readAllLines(Path.of(c.getResource(c.getSimpleName() + "-non-tested-functions.txt").toURI()));


It would be great if it's possible get all the functions from FunctionRegistry:
Something like: https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java#L2199

Can also be done at a later point and not as part of this PR.

SqlFunctionRegistry is not available in integration tests (can add it as a dependency, but I don't think we really need it). I get the list of the functions from SHOW FUNCTIONS see here, that achieves pretty much the same that SqlFunctionRegistry would (for more see the last paragraph here). The purpose of the NON_TESTED_FUNCTIONS list is to identify functions added after writing this test class which likely we should think about adding to the tested function list / non tested function list (one-liner). Newly added = SHOW FUNCTIONS \ FUNCTIONS_CALLS \ NON_TESTED_FUNCTIONS should be an empty set. The NON_TESTED_FUNCTIONS list should shrink later once the datetime, ... functions are also added to the tested function list (separate PR).

astefan

LGTM

…68379) Fixed the inconsistencies regarding NULL argument handling. NULL literal vs NULL field value as function arguments in some case resulted in different function return values. Functions should return with the same value no matter if the argument(s) came from a field or from a literal. The introduced integration test tests if function calls with same argument values (regardless of literal/field) will return with the same output (also checks if newly added functions are added to the testcases). Fixed the following functions: * Insert: NULL start, length and replacement arguments (as fields) also result in NULL return value instead of returning the input. * Locate: NULL pattern results in NULL return value, NULL optional start argument handled the same as missing start argument * Replace: NULL pattern and replacement results in NULL instead of returning the input * Substring: NULL start or length results in NULL instead of returning the input Fixes elastic#58907

…68684) Fixed the inconsistencies regarding NULL argument handling. NULL literal vs NULL field value as function arguments in some case resulted in different function return values. Functions should return with the same value no matter if the argument(s) came from a field or from a literal. The introduced integration test tests if function calls with same argument values (regardless of literal/field) will return with the same output (also checks if newly added functions are added to the testcases). Fixed the following functions: * Insert: NULL start, length and replacement arguments (as fields) also result in NULL return value instead of returning the input. * Locate: NULL pattern results in NULL return value, NULL optional start argument handled the same as missing start argument * Replace: NULL pattern and replacement results in NULL instead of returning the input * Substring: NULL start or length results in NULL instead of returning the input Fixes #58907 (cherry-pick from a3dbdae)

palesz added >bug :Analytics/SQL SQL querying v8.0.0 v7.12.0 v7.11.1 labels Feb 2, 2021

palesz requested review from costin, astefan, bpintea and matriv February 2, 2021 14:23

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label Feb 2, 2021

palesz commented Feb 2, 2021

View reviewed changes

Andras Palinkas added 2 commits February 2, 2021 18:08

Revert the LOCATE breaking change, checkstyle fixes

baa147c

Merge remote-tracking branch 'origin/master' into all-sql-fn-null

1e8caf5

palesz added the >breaking label Feb 2, 2021

Merge branch 'master' into string-fn-null-handling

0f87494

costin reviewed Feb 3, 2021

View reviewed changes

Andras Palinkas added 6 commits February 3, 2021 15:51

Extracted the non-tested function list into a separate file (to keep …

8c41ce3

…spotless happy and the code short)

Doc updates on LOCATE and CONCAT edge cases.

7b8de11

Merge remote-tracking branch 'origin/master'

5cc3625

License changes

ad0ce06

Merge remote-tracking branch 'origin/master' into all-sql-fn-null

900ed2d

Merge remote-tracking branch 'palesz/string-fn-null-handling' into al…

968fc33

…l-sql-fn-null

Andras Palinkas added 2 commits February 3, 2021 16:47

Making spotless happy

4e20cbf

Merge remote-tracking branch 'origin/master' into all-sql-fn-null

9fb6922

astefan reviewed Feb 3, 2021

View reviewed changes

costin approved these changes Feb 4, 2021

View reviewed changes

bpintea approved these changes Feb 4, 2021

View reviewed changes

palesz requested a review from astefan February 4, 2021 16:45

astefan reviewed Feb 4, 2021

View reviewed changes

matriv approved these changes Feb 5, 2021

View reviewed changes

Andras Palinkas added 2 commits February 5, 2021 11:18

Addressed feedback

c3e1df7

Merge remote-tracking branch 'origin/master' into all-sql-fn-null

1731a5e

astefan approved these changes Feb 5, 2021

View reviewed changes

palesz merged commit a3dbdae into elastic:master Feb 5, 2021

palesz added the backport pending label Feb 8, 2021

palesz removed backport pending v7.11.1 labels Feb 8, 2021

palesz deleted the string-fn-null-handling branch February 8, 2021 21:38

pgomulka mentioned this pull request Feb 11, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

This was referenced Jun 16, 2021

[DOCS] Document null/edge case handling for SQL functions #74193

Closed

[DOCS] SQL: Document null handing for string functions #74201

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		false \|83
		true \|13

SQL: Test and fix the NULL handling of the String functions #68379

SQL: Test and fix the NULL handling of the String functions #68379

Conversation

palesz commented Feb 2, 2021 • edited Loading

elasticmachine commented Feb 2, 2021

palesz Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palesz Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin commented Feb 3, 2021

matriv commented Feb 3, 2021

palesz commented Feb 3, 2021

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv commented Feb 4, 2021 • edited Loading

costin left a comment

Choose a reason for hiding this comment

palesz commented Feb 4, 2021

palesz commented Feb 4, 2021

bpintea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

palesz commented Feb 2, 2021 •

edited

Loading

palesz Feb 2, 2021 •

edited

Loading

palesz Feb 4, 2021 •

edited

Loading

matriv commented Feb 4, 2021 •

edited

Loading